C++: Fix FP in PointlessComparison due to preprocessor#1165
Conversation
Reported by an LGTM customer here: https://discuss.lgtm.com/t/2-false-positives-in-c-for-comparison-is-always-same/1943. Even though the comparison is pointless in the preprocessor configuration in effect during extraction, it is not pointless in other preprocessor configurations. Similar to ExprHasNoEffect, we'll now exclude results in functions that contain preprocessor-excluded code. I factored the similar code already used in ExprHasNoEffect in a non-recursive version into Preprocessor.qll, leaving the recursive version in ExprHasNoEffect.ql. I believe the recursive version is too aggressive for PointerlessComparison, which does no interprocedural analysis.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
A few comments, otherwise LGTM.
| pbd.getNext() instanceof PreprocessorElse or | ||
| pbd instanceof PreprocessorElse or | ||
| pbd.getNext() instanceof PreprocessorElif or | ||
| pbd instanceof PreprocessorElif |
There was a problem hiding this comment.
I appreciate that this was the existing code, but I don't think it covers the case where the definition we're looking at is in the final part of an if-else chain. Perhaps it should be something like pbd.getIf().getNext() != pbd.getEndif()?
There was a problem hiding this comment.
Isn't that covered by pbd instanceof PreprocessorElse?
There was a problem hiding this comment.
Good point (for some reason I was only reading the even numbered lines)
| predicate containsDisabledCode(Function f) { | ||
| // `f` contains a preprocessor branch that was not taken | ||
| exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | | ||
| functionLocation(f, file, fBlockStartLine, fBlockEndLine) and |
There was a problem hiding this comment.
Indentation needs fixing here.
| pbd instanceof PreprocessorElse | ||
| ) | ||
| ) or | ||
| // recurse into function calls |
There was a problem hiding this comment.
Did you mean to delete this case from this version?
There was a problem hiding this comment.
Yes, I did. Fixed.
3 participants
Reported by an LGTM customer here: https://discuss.lgtm.com/t/2-false-positives-in-c-for-comparison-is-always-same/1943.
Even though the comparison is pointless in the preprocessor configuration in effect during extraction, it is not pointless in other preprocessor configurations. Similar to ExprHasNoEffect, we'll now exclude results in functions that contain preprocessor-excluded code. I factored the similar code already used in ExprHasNoEffect in a non-recursive version into Preprocessor.qll, leaving the recursive version in ExprHasNoEffect.ql. I believe the recursive version is too aggressive for PointerlessComparison, which does no interprocedural analysis.